refactor: use &'static [PReg] instead of Vec<PReg>#208
refactor: use &'static [PReg] instead of Vec<PReg>#208JonasKruckenberg wants to merge 5 commits intobytecodealliance:mainfrom
&'static [PReg] instead of Vec<PReg>#208Conversation
| env.preferred_regs_by_class[RegClass::Int as usize].clone(), | ||
| env.preferred_regs_by_class[RegClass::Float as usize].clone(), | ||
| env.preferred_regs_by_class[RegClass::Vector as usize].clone(), | ||
| Vec::from(env.preferred_regs_by_class[RegClass::Int as usize]), |
There was a problem hiding this comment.
don't love this, open for better ideas
There was a problem hiding this comment.
Maybe use .to_owned() instead of Vec::from?
There was a problem hiding this comment.
hmm, yeah doesn't really change anything about it but might look nicer
Co-authored-by: bjorn3 <17426603+bjorn3@users.noreply.github.com>
|
Hmm seems libfuzz doesn't like |
interestingly I locally can't replicate the CI fail |
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
A few comments below -- thanks for working on this! I think the general shape is right, I just have some thoughts on the way the fuzzing testcase generation is working (we need to avoid leaks).
| /// will have very few of and generally want to keep around for their entire lifetime. | ||
| /// In order to make static initialization easier the registers lists are static slices instead | ||
| /// of `Vec`s. If your use case depends on dynamically creating the registers lists, consider | ||
| /// [`Vec::leak`]. |
There was a problem hiding this comment.
I'd prefer not to give this suggestion in the doc-comment: since it's parameterized on a lifetime (rather than hardcoding 'static), it's possible to build a MachineEnv dynamically that is a borrowed view on owned Vecs, basically giving the same capabilities as before. This is fairly idiomatic in Rust, so I don't think we need a special note or recommendation.
There was a problem hiding this comment.
ah yeah good catch, that was from before I allowed any lifetimes. This notice doesn't make much sense anymore
| ] | ||
| }) | ||
| .collect(); | ||
| let fixed_stack_slots = Vec::leak( |
There was a problem hiding this comment.
I think we need to avoid Vec::leak here: fuzzing will continually generate new functions with machine envs, and we cannot allocate indefinitely. Can you build a static initializer, since the range (32..63) is static?
| .collect(), | ||
| vec![], | ||
| vec![], | ||
| Vec::leak( |
There was a problem hiding this comment.
We definitely can't leak while fuzzing -- can we instead have a few static MachineEnvs in scope, and pick one of them? We don't necessarily need to fuzz with arbitrary no_of_regs, just with a few values that are interesting -- the two extremes of its range and a few in the middle, for example.
3 participants
This PR makes
MachineEnvuse&'static [PReg]instead ofVec<PReg>to make it possible to placeMachineEnvintostatics as part of bytecodealliance/wasmtime#8489.Edit: as it turns out serialization makes this not quite as trivial,
serdecan't deserialize into slices (had forgotten that) so I introduced a newSerializableMachineEnvtype in the spirit ofSerializableFunctionto let consumers still serialize a machine env if required.related: #177.